Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for external auth providers in code search #6919

Merged
merged 8 commits into from
Feb 14, 2025

Conversation

pkukielka
Copy link
Contributor

@pkukielka pkukielka commented Feb 3, 2025

Changes

This PR adds agent endpoint which allow us to fetch auth headers, which in turn we need to deliver to the sourcegraph search webview.

Webview changes are on a separate PR: pkukielka/sourcegraph-public-snapshot#1

Test plan

Setup:

  1. Clone [email protected]:pkukielka/sourcegraph-public-snapshot.git and checkout pkukielka/add-external-auth-providers-support branch.
  2. export CODE_SEARCH_DIR_OVERRIDE=/path/to/pkukielka/sourcegraph-public-snapshot
  3. Checkout this PR (pkukielka/ext-auth-fix-search branch) in Cody project dir
  4. cd jetbrains
  5. Run ./gradlew customRunIde -PforceCodeSearchBuild=true -PforceAgentBuild=true
  6. Add external auth provider configuration:
{
  "cody.override.serverEndpoint":  "https://piotr-kukielka.sgdev.dev",

  "cody.auth.externalProviders": [
    {
      "endpoint": "https://piotr-kukielka.sgdev.dev",
      "executable": {
        "commandLine": ["echo '{ \"headers\": { \"Authorization\": \"token sgp_local_YOUR_TOKEN\" } }'"],
        "shell": "/bin/bash"
      }
    },

Test

  1. Hit Option + S to open sourcegraph search
  2. Search for cody repo and click Open in Browser
  3. Make sure https://piotr-kukielka.sgdev.dev instance is opened
  4. Comment out "cody.override.serverEndpoint" line and then switch to other account, e.g. on https://sg02.sourcegraphcloud.com/ (this one will authenticate using token), save settings
    1. Search for cody repo and click Open in Browser
  5. Make sure https://sg02.sourcegraphcloud.com/ instance is opened

Reproducing failing fetches:

  1. Edit config by changing commandLine line to:
    "commandLine": ["echo '{ \"headers\": { \"X-Forwarded-User\": \"SomeUser\" } }'"],
  2. Open Find Actions: (Ctrl+Shift+A / ⌘⇧A)
  3. Search for "Registry..." and open it
  4. Find option ide.browser.jcef.debug.port
  5. Change the default value to an open port (we use 9222)
  6. Restart IDE
  7. Hit Option + S to open sourcegraph search
  8. Notice sourcegraph search is not loading anymore
  9. Go to http://localhost:9222/ and click on Sourcegraph link to inspect the failures in the devtools

@pkukielka pkukielka marked this pull request as draft February 3, 2025 16:14
@pkukielka pkukielka force-pushed the pkukielka/ext-auth-fix-search branch from dfc3ef0 to aa904c1 Compare February 6, 2025 15:46
@pkukielka pkukielka changed the title Pass custom headers to the search plugin Add support for external auth providers in code search Feb 6, 2025
@pkukielka pkukielka force-pushed the pkukielka/ext-auth-fix-search branch from aa904c1 to 4a47767 Compare February 7, 2025 09:04
Copy link
Contributor

@mkondratek mkondratek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good. I am going to test it

@@ -343,7 +343,9 @@ tasks {
return destinationDir
}

val sourcegraphDir = unzipCodeSearch()
val codeSearchDirOverride = System.getenv("CODE_SEARCH_DIR_OVERRIDE")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

}
}
return {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously we had a console.error. do we need it? or is {} a valid option?

@@ -19,8 +19,6 @@ class CodySettingChangeListener(project: Project) : ChangeListener(project) {
CodySettingChangeActionNotifier.TOPIC,
object : CodySettingChangeActionNotifier {
override fun afterAction(context: CodySettingChangeContext) {
// Notify JCEF about the config changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we need to refreshConfiguration now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That config was already not containing any info useful for sourcegraph search.
We care about endpoint and headers.
Endpoint change is signalled in CodyAuthService.
Headers are fetched every time so we do not need to notify about any change in their case.

Comment on lines 32 to 38
} catch (Exception ignored) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it safe to ignore it and return false? does it make sense to log/warn here?

@@ -319,10 +343,9 @@ val pnpmPath =
}

tasks {
val codeSearchCommit = "9d86a4f7d183e980acfe5d6b6468f06aaa0d8acf"
val codeSearchCommit = "048679a2e7f2a2d94a49c46bc972c954cb2b5bac"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pkukielka pkukielka force-pushed the pkukielka/ext-auth-fix-search branch from 2402bc9 to 33d8a88 Compare February 13, 2025 10:12
@pkukielka pkukielka marked this pull request as ready for review February 13, 2025 10:13
@pkukielka pkukielka force-pushed the pkukielka/ext-auth-fix-search branch from 33d8a88 to 3576de9 Compare February 13, 2025 10:17
@@ -41,3 +41,4 @@ jobs:
popd > /dev/null
env:
PUBLISH_TOKEN: ${{ secrets.JETBRAINS_MARKETPLACE_PUBLISH_TOKEN }}
GITHUB_TOKEN: ${{ secrets.PRIVATE_SG_TOKEN }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit inline, looks great! What a load of work that turned out to be.

assert(output.parentFile.mkdirs()) { output.parentFile }
Files.copy(URL(url).openStream(), output.toPath())

// Optional: Retrieve the GitHub token if needed for authorization (require private repo access)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Optional: Retrieve the GitHub token if needed for authorization (require private repo access)
// Optional: Retrieve the GitHub token if needed for authorization (requires sourcegraph/sourcegraph repo access)

assert(output.parentFile.mkdirs()) { output.parentFile }
Files.copy(URL(url).openStream(), output.toPath())

// Optional: Retrieve the GitHub token if needed for authorization (require private repo access)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document a bit about this wrinkle in CONTRIBUTING.md.

export async function addAuthHeaders(auth: AuthCredentials, headers: Headers, url: URL): Promise<void> {
await getAuthHeaders(auth, url).then(authHeaders => {
for (const [key, value] of Object.entries(authHeaders)) {
headers.set(key, value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do Object.assign for this and avoid the loop.

@pkukielka pkukielka force-pushed the pkukielka/ext-auth-fix-search branch from 539846b to b52c2f4 Compare February 13, 2025 17:32
@pkukielka pkukielka force-pushed the pkukielka/ext-auth-fix-search branch from b52c2f4 to ca403b4 Compare February 14, 2025 08:31
@pkukielka pkukielka merged commit 86d940a into main Feb 14, 2025
20 of 21 checks passed
@pkukielka pkukielka deleted the pkukielka/ext-auth-fix-search branch February 14, 2025 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants